Skip to content

Stop doing full-array work at open to answer approximate questions - #23

Merged
cboulay merged 2 commits into
feature/apply-stored-conversionfrom
perf/faster-slicer-init
Aug 29, 2026
Merged

Stop doing full-array work at open to answer approximate questions#23
cboulay merged 2 commits into
feature/apply-stored-conversionfrom
perf/faster-slicer-init

Conversation

@cboulay

@cboulay cboulay commented Aug 29, 2026

Copy link
Copy Markdown
Member

Stacked on #22 — review that first, and this diff is just the two commits on top.

Opening an NWBSlicer did several full passes over a recording's timestamps to compute statistics that never needed that precision. On a 5-stream 5.7 GB recording:

before after
open, dejitter cache warm 218 ms 119 ms
open, cache cold (first sight of a file) 250 ms + 1.02 s reconstruction 119 ms + 0.57 s

The cold number is the one that matters for batch analysis, where every file is seen for the first time.

What was slow

np.median over every inter-sample interval — 45 ms a go, several times per stream. infer_nominal_rate uses the median only to centre a trim window; the estimate it returns is a mean over the intervals that survive. Partitioning 7 million floats to place a window is precision nobody asked for. clockmodel's _nominal_period and _auto_gap_threshold do the same thing on the same arrays.

np.var computed twice to evaluate one or. Short-circuiting meant the second full pass only ran when the first comparison failed — which is exactly the irregular-stream case, i.e. the slow one.

electrodes.table.to_dataframe() — 12 ms per electrical series. It materialises every column the writer stored (position, group, filtering, …) into pandas, and resolves an object reference per row, in order to read one column.

Why subsampling the median is exact here

Timestamps arrive on a device's quantisation grid, so their differences cluster onto that grid and the median snaps to a grid point that a stride cannot move. On a real 7.09M-interval stream, strides of 16 through 1024 all return 3.3301000002e-05, and infer_nominal_rate returns 30147.9172 from every one of them.

Verified end to end on that recording: all five streams reconstruct bit-identical dejittered timestamps, and the gap threshold matches to the last digit. Two of those streams reach the inference path with non-trivial rates (30137.773554857125 and 30000.082189805245), so this isn't only exercising the easy cases.

The helper is named interval_median and lives in util.py, which imports nothing local — clockmodel can't import from slicer, since slicer imports clockmodel. The name states what licenses the shortcut: intervals inherit a grid, and other quantities don't.

What deliberately did not change

_auto_gap_threshold keeps its percentile on the full array. Subsampling the whole function is 16x faster and moves the threshold 1.4% (0.0133720.013557), because a 99.9th percentile over 200k values rests on their top 200 instead of on 7,000. That threshold decides which interval jumps count as real gaps, so moving it re-segments the stream and re-times its samples — a behaviour change wearing a speedup's clothes. A test asserts the asymmetry so nobody finishes the job later.

through_c's median is untouched for the same reason: it averages fit residuals, which are continuous and have no grid to snap to.

Tests

tests/test_slicer_perf.py pins equivalence, not speed — interval_median against np.median across the threshold, _nominal_period and _auto_gap_threshold against themselves computed with a full median, and inferred rates unchanged. Because the trim is the only thing that makes a mean usable here, one test gives a stream 400 dropped-packet holes and asserts it still reports 30 kHz rather than an average including the holes. Another pins that the label column read honours the electrodes region, since a series may reference a subset of the table.

The fallback for a table with no label column now names channels from the id column rather than a DataFrame index. That is not a change — the index to_dataframe() built was the id column — but it has to be explicit now that the DataFrame is gone, because positional indices coincide with ids only when a series references the whole table in order.

256 tests pass; ruff clean.

What's left, for the record

After this, opening is ~119 ms, of which 108 ms is inside pynwb.read() building Python objects. About 24 ms of that is pynwb#2253 (submitted upstream): FeatureExtraction.__init__ copies times into a Python list, reading the backing dataset one element at a time — 23,630 separate h5py reads on this file, 84x slower than one bulk read. The rest is hdmf's object construction and not ours to fix cheaply.

Chunk iteration was profiled separately and needs no work: per-chunk CPU in our code is ~0.07 ms against an 11–16 ms chunk, and raw h5py forward reads run at the same 1.09 GB/s the iterator achieves. It is disk-bound, not us.

🤖 Generated with Claude Code

NWBSlicer construction on a 5-stream 5.7 GB recording: 218 ms -> 119 ms
with the dejitter cache warm, 250 -> 119 cold. Two thirds of what is left
is inside pynwb's read(), building Python objects for the file, which is
not ours to fix cheaply.

np.median over every interval, 45 ms per stream. The median here is never
the answer -- infer_nominal_rate returns a mean over the intervals that
survive a trim window, and the median only says where to centre that
window. Partitioning 7 million floats to place a window is precision
nobody asked for.

It is not even approximate in practice. Timestamps arrive on a
quantisation grid, so the median snaps to a grid point that a stride
cannot move: strides of 16 through 1024 over a real 7.09M-interval stream
all give 3.3301000002e-05. Verified end to end that every stream in that
recording reports a bit-identical rate with the subsample as with the full
median, including the two _device_ts streams that actually reach the
inference path at 30137.773554857125 and 30000.082189805245.

Strided rather than head-of-array, so a stream whose intervals drift
between its start and its end is represented across the estimate instead
of by whichever end happened to be read first.

np.var was also computed twice to evaluate one `or`. Short-circuiting
meant the second pass only ran when the first comparison failed -- which
is exactly the irregular-stream case, i.e. the slow one.

electrodes.table.to_dataframe(), 12 ms per electrical series. It
materializes every column the writer stored -- position, group, filtering
-- into pandas so we can read one of them, and it drags in a reference
resolution per row on the way. Indexing the label column directly is the
same values for a thirtieth of the work.

The fallback for a table with no label column now names channels from the
id column rather than from a DataFrame index. That is not a change: the
index to_dataframe() built was the id column. Making it explicit is what
keeps it correct now that the DataFrame is gone, since positional indices
coincide with ids only when a series references the whole table in order.

Tests pin equivalence rather than speed: _fast_median against np.median
across the threshold, inferred rates unchanged, and -- because the trim is
the only reason a mean is usable here -- that a stream with 400
dropped-packet holes still reports 30 kHz rather than an average including
the holes.
Cold open -- the first time a file is seen, which for batch analysis is
every file -- spent ~0.67 s taking medians over full recordings inside
clockmodel.py, which the earlier slicer-side fix did not reach.
Reconstruction time for a 5-stream 5.7 GB recording: 1.02 s -> 0.57 s,
measured by alternating the two implementations so drift hits both.

_fast_median moves to util.py as interval_median, since clockmodel cannot
import from slicer (slicer imports clockmodel) and util imports neither.
The rename says what the shortcut is licensed by: these are medians of
inter-sample intervals, and intervals inherit the device's quantisation
grid, so the median snaps to a grid point a stride cannot move. Verified
on a real recording that all five streams reconstruct bit-identical
timestamps and the gap threshold matches to the last digit.

_auto_gap_threshold takes the shortcut for its median and deliberately not
for its percentile. Subsampling the whole function is 16x faster and moves
the threshold 1.4%, because a 99.9th percentile over 200k values rests on
their top 200 instead of on 7000. That threshold decides which jumps are
real gaps, so moving it re-segments the stream and re-times its samples --
a behaviour change wearing a speedup's clothes. A test asserts the
asymmetry so nobody 'finishes the job' later.

through_c's median is left alone for the same reason: it averages fit
residuals, which are continuous and have no grid to snap to.
@cboulay
cboulay force-pushed the perf/faster-slicer-init branch from 1620b7d to 17ac757 Compare August 29, 2026 06:09
@cboulay
cboulay merged commit efb1176 into dev Aug 29, 2026
14 checks passed
@cboulay
cboulay deleted the perf/faster-slicer-init branch August 29, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant